wolfTPM2: harden PCR/hash wrapper validation#554
Conversation
- ReadPCR: reject an empty response (count == 0) before dereferencing digests[0] - HashStart: reject any non-hash algorithm (digest size 0), not just TPM_ALG_NULL - ExtendPCR / ResetPCR: bound pcrIndex to [PCR_FIRST, PCR_LAST], matching ReadPCR
aidangarske
left a comment
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: COMMENT
Findings: 1 total — 1 posted, 0 skipped
1 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review+review-security] New wrapper validation paths are not covered by regression tests —
src/tpm2_wrap.c:6695-7725
Review generated by Skoll
| @@ -6695,6 +6695,10 @@ int wolfTPM2_ResetPCR(WOLFTPM2_DEV* dev, int pcrIndex) | |||
| { | |||
There was a problem hiding this comment.
🟠 [Medium] New wrapper validation paths are not covered by regression tests · Missing Tests
The PR adds defensive validation to four public PCR/hash wrappers: wolfTPM2_ResetPCR and wolfTPM2_ExtendPCR now reject out-of-range PCR indexes (return BAD_FUNC_ARG), wolfTPM2_ReadPCR now rejects a successful empty PCR response (pcrValues.count == 0 -> TPM_RC_FAILURE) before reading digests[0], and wolfTPM2_HashStart now rejects any algorithm for which TPM2_GetHashDigestSize(hashAlg) == 0. These are observable local wrapper contracts, but the diff changes only src/tpm2_wrap.c and adds no regression tests. The existing tests only exercise happy paths (PCR reset/extend success, HashStart with TPM_ALG_SHA256) and do not assert the new negative paths: invalid PCR indexes, non-hash HashStart input, or an empty PCR read response. Severity views differ: the review mode rated this Medium while review-security rated it Info; keeping the stricter Medium. This is not a blocking security issue in the changed code, but regression coverage would prevent the behavior from being accidentally relaxed later.
Fix: Add focused regression tests in tests/unit_tests.c for the new negative paths: wolfTPM2_ResetPCR(&dev, PCR_LAST + 1) and wolfTPM2_ExtendPCR(&dev, PCR_LAST + 1, ...) returning BAD_FUNC_ARG, wolfTPM2_HashStart(&dev, &hash, TPM_ALG_RSA, NULL, 0) returning BAD_FUNC_ARG, and an empty PCR read response (pcrValues.count == 0 -> TPM_RC_FAILURE) using a harness/backend configuration that can produce or mock that TPM response.
3 participants
Defensive-validation improvements in the PCR/hash wrappers:
reading digests[0] uninitialized.
TPM2_GetHashDigestSize(), not only TPM_ALG_NULL.
No API change; builds clean.